-
Notifications
You must be signed in to change notification settings - Fork 841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate EuiTable and a bunch of other stuff to TS #2212
Migrate EuiTable and a bunch of other stuff to TS #2212
Conversation
…porting circular dependencies
@chandlerprall I've resolved most of the issues with the generated |
To be more specific - the generated definitions in
contain this line:
This is odd because this import doesn't appear in the original source, in:
In the final
which I thought ought to resolve, but doesn't. If I add It looks a bit like something (maybe TypeScript, I don't know) is spotting that the type of |
Looks like |
@pugnascotia I saw that same oddity. |
I wiped out |
New question: Do you use |
@pugnascotia See pugnascotia#3 for some additional things. I think these get us to a point where no unnecessary changes to Kibana will need to happen. |
Most of the additional exports are for Kibana, which imports them directly.
CI is now green! Any chance of a review now, @chandlerprall / @thompsongl? |
I'll take another pass. I do know there'll be a merge conflict once #1933 gets in, but it shouldn't be too bad. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ready to me. We'll have a good deal of work to do to get Kibana's type check to pass, but all the errors look like legitimate configurations that need changing.
@snide, given we're a week out from FF and the last release is still blocked, do we want to merge this now?
@thompsongl and i discussed offline. We're going to do an EUI release before this lands, with the intention of it being the final release for Kibana 7.4. When that's done (let's just say by tomorrow) we can merge this in and set it up for 7.5. |
Ok, let me know when I’m good to merge. |
@pugnascotia Just an update. We need one more day. We have some complication with EUI PRs hitting kibana downstream and need a couple important PRs to get in before this one (since we don't want to deal with fallout from this in Kibana). I'll post an update tomorrow, but should be OK to merge then. |
@pugnascotia You're now safe to merge. Thanks! |
…migrate-all-the-ts-things
* WIP - migrating all kinds of things to TS * More WIP * Various type fixes * Fix tests * Change quirky syntax * Update changelog * Fix ts->proptype script by 1. not duplicating PropTypes import 2. supporting circular dependencies * Type fixes * Remove old type defs and fix some types * Fix some more old type def cruft * more exports * remove SPACING type
Summary
This PR converts the fundamental table components to TypeScript. As a consequence, it also converts a number of other components; popover, buttons, pagination, outside click detector, focus trap, context menu, and panel.
@chandlerprall I'm getting an error with the prop type extractor. Could you take a look? With it disabled in the babel config, all the tests were passing (apart from a couple of flex tests that rely on the extract prop types to be present).
Checklist